Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Update ClientOptions Based on HTTP and WS #265

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Fix Update ClientOptions Based on HTTP and WS #265

merged 1 commit into from
Apr 5, 2024

Conversation

davidvonthenen
Copy link
Contributor

@davidvonthenen davidvonthenen commented Apr 5, 2024 •
edited by coderabbitai bot
Loading

I have been thinking about this a lot and I think separating out HTTP vs WS is probably the better thing to do for ClientOptions. As an example, the KeepAlive (currently) has no meaning for HTTP (yes, I know HTTP does actually support keeping a connection open). Just seems like this is the best way to avoid "overloaded" meanings on this because the object is entirely different.

Other changes:

  • Only expose the most common options in each of the constructors as to avoid calling constructors with Constructor(null, null, null, null, something)

Summary by CodeRabbit

  • Refactor
    • Enhanced initialization of client options across various tests for consistency, specifically for on-premise configurations.
  • Bug Fixes
    • Fixed custom headers setup in HTTP client configuration by adjusting initialization in unit tests.
  • Chores
    • Removed unused KeepAlive property and associated logic from client options to streamline HTTP client setup.
  • New Features
    • Improved user interaction in the prerecorded file example by adding a prompt to exit the program.

@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 5, 2024 •
edited
Loading

Walkthrough

The updates focus on enhancing the configuration for the on-premise option across various client tests in the Deepgram SDK, alongside removing the KeepAlive property and its related logic from the DeepgramHttpClientOptions. These changes streamline the initialization of client options and adjust the HTTP client configuration, particularly for on-premise deployments, while simplifying the options model by eliminating less relevant properties.

Changes

Files Change Summary
.../UnitTests/ClientTests/*.cs Set OnPrem to true using object initializer in client options.
.../UnitTests/HttpExtensionsTests/HttpClientExtensionTests.cs Updated _clientOptions initialization by adjusting custom headers setup.
Deepgram/Abstractions/AbstractRestClient.cs Removed logging of KeepAlive option.
Deepgram/Models/Authenticate/v1/DeepgramHttpClientOptions.cs Removed KeepAlive property and updated constructor signature.
Deepgram/Models/Authenticate/v1/DeepgramWsClientOptions.cs Updated constructor signature and modified sections.
examples/prerecorded/file/Program.cs Added prompt for user to press any key to exit after displaying JSON response.

Possibly related issues

  • Further HTTP/REST Refactoing #234: The refactoring mentioned in the PR could contribute to the thread safety improvements and customization of HTTP parameters for the query string as outlined in the issue. The removal of KeepAlive and adjustments in client option initializations align with the broader goal of refining HTTP/REST client configurations.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot]
coderabbitai bot reviewed Apr 5, 2024
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 138a52d and 186c33b.
Files selected for processing (11)
  • Deepgram.Tests/UnitTests/ClientTests/AbstractRestClientTests.cs (1 hunks)
  • Deepgram.Tests/UnitTests/ClientTests/AnalyzeClientTests.cs (1 hunks)
  • Deepgram.Tests/UnitTests/ClientTests/ManageClientTest.cs (1 hunks)
  • Deepgram.Tests/UnitTests/ClientTests/OnPremClientTests.cs (1 hunks)
  • Deepgram.Tests/UnitTests/ClientTests/PrerecordedClientTests.cs (1 hunks)
  • Deepgram.Tests/UnitTests/ClientTests/SpeakClientTests.cs (1 hunks)
  • Deepgram.Tests/UnitTests/HttpExtensionsTests/HttpClientExtensionTests.cs (3 hunks)
  • Deepgram/Abstractions/AbstractRestClient.cs (1 hunks)
  • Deepgram/Models/Authenticate/v1/DeepgramHttpClientOptions.cs (2 hunks)
  • Deepgram/Models/Authenticate/v1/DeepgramWsClientOptions.cs (2 hunks)
  • examples/prerecorded/file/Program.cs (1 hunks)
Files skipped from review due to trivial changes (1)
  • Deepgram/Abstractions/AbstractRestClient.cs
Additional comments not posted (14)
examples/prerecorded/file/Program.cs (1)

47-48: The addition of a prompt for the user to press any key to exit enhances the usability of the program by making the execution flow clearer.

Deepgram/Models/Authenticate/v1/DeepgramHttpClientOptions.cs (2)

56-59: The removal of sections like Prerecorded, Manage, and Analyze, and the addition of the Speak section, along with the constructor changes, align with the PR's objectives of refining client options handling. Ensure that the removal of these sections does not impact other functionalities.

63-63: The constructor's simplification by removing the keepAlive parameter and related logic is a positive change, making the client options more intuitive and aligned with the PR's objectives.

Deepgram.Tests/UnitTests/HttpExtensionsTests/HttpClientExtensionTests.cs (3)

73-73: The update to _clientOptions initialization in the test method reflects the changes made to the DeepgramHttpClientOptions constructor, ensuring the tests remain valid.

98-98: Correctly updated _clientOptions initialization in this test method to align with the simplified DeepgramHttpClientOptions constructor.

125-125: The adjustment in _clientOptions initialization here is consistent with the modifications to the DeepgramHttpClientOptions constructor, ensuring the test's accuracy.

Deepgram/Models/Authenticate/v1/DeepgramWsClientOptions.cs (2)

53-53: The addition of the Speak section and the removal of other sections in the DeepgramWsClientOptions class align with the PR's objectives of refining client options handling. Ensure that the removal of these sections does not impact other functionalities.

59-59: The constructor's simplification by removing the apiVersion parameter and related logic is a positive change, making the WebSocket client options more intuitive and aligned with the PR's objectives.

Deepgram.Tests/UnitTests/ClientTests/OnPremClientTests.cs (1)

20-23: The update to _clientOptions initialization in the Setup method, including setting the OnPrem property to true, is correctly implemented and aligns with the PR's objectives of refining client options handling.

Deepgram.Tests/UnitTests/ClientTests/SpeakClientTests.cs (1)

20-23: The update to _clientOptions initialization in the Setup method, including setting the OnPrem property to true, is correctly implemented and aligns with the PR's objectives of refining client options handling.

Deepgram.Tests/UnitTests/ClientTests/AbstractRestClientTests.cs (1)

21-24: The update to _clientOptions initialization in the Setup method, including setting the OnPrem property to true, is correctly implemented and aligns with the PR's objectives of refining client options handling.

Deepgram.Tests/UnitTests/ClientTests/AnalyzeClientTests.cs (1)

20-23: The update to _options initialization in the Setup method, including setting the OnPrem property to true, is correctly implemented and aligns with the PR's objectives of refining client options handling.

Deepgram.Tests/UnitTests/ClientTests/PrerecordedClientTests.cs (1)

20-23: Ensure that tests adequately cover both on-premises (OnPrem = true) and cloud scenarios, given the change in DeepgramHttpClientOptions.

Deepgram.Tests/UnitTests/ClientTests/ManageClientTest.cs (1)

22-25: Ensure that tests adequately cover both on-premises (OnPrem = true) and cloud scenarios, given the change in DeepgramHttpClientOptions.

jpvajda
jpvajda approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@jpvajda jpvajda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidvonthenen davidvonthenen merged commit 856fccb into deepgram:main Apr 5, 2024
4 checks passed
@davidvonthenen davidvonthenen deleted the consolidate-client-options branch April 5, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@jpvajda jpvajda jpvajda approved these changes

@lukeocodes lukeocodes Awaiting requested review from lukeocodes

@SandraRodgers SandraRodgers Awaiting requested review from SandraRodgers

@CopperBeardy CopperBeardy Awaiting requested review from CopperBeardy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidvonthenen @jpvajda